-
Notifications
You must be signed in to change notification settings - Fork 35
Update GetBinaryOperator to GetOperator
#394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update GetBinaryOperator to GetOperator
#394
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #394 +/- ##
==========================================
- Coverage 70.87% 70.85% -0.02%
==========================================
Files 9 9
Lines 3533 3538 +5
==========================================
+ Hits 2504 2507 +3
- Misses 1029 1031 +2
|
3c7449a to
5f7c378
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe address the clang-tidy issue.
371ef22 to
709371e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
| OP_Coawait, | ||
| }; | ||
|
|
||
| enum OperatorKind { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: enum 'OperatorKind' uses a larger base type ('unsigned int', size: 4 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]
enum OperatorKind {
^
lib/Interpreter/CppInterOp.cpp
Outdated
| if (auto* FD = llvm::dyn_cast<FunctionDecl>(D)) { | ||
| if (FD->isOverloadedOperator()) { | ||
| switch (FD->getOverloadedOperator()) { | ||
| #define OVERLOADED_OPERATOR(Name, Spelling, Token, Unary, Binary, \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function-like macro 'OVERLOADED_OPERATOR' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]
#define OVERLOADED_OPERATOR(Name, Spelling, Token, Unary, Binary, \
^There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That type of check should be disabled in our clang-tidy config..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
| kUnary = 0b001, | ||
| kBinary = 0b010, | ||
| kBoth = 0b011, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we won't or these, 1,2,3 should be fine...
| OP_Coawait, | ||
| }; | ||
|
|
||
| enum OperatorKind { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| enum OperatorKind { | |
| enum OperatorArity { |
709371e to
8277fe9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
| OP_Coawait, | ||
| }; | ||
|
|
||
| enum OperatorArity { kUnary = 1, kBinary, kBoth }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: enum 'OperatorArity' uses a larger base type ('unsigned int', size: 4 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]
enum OperatorArity { kUnary = 1, kBinary, kBoth };
^
lib/Interpreter/CppInterOp.cpp
Outdated
|
|
||
| void GetBinaryOperator(TCppScope_t scope, enum BinaryOperator op, | ||
| std::vector<TCppFunction_t>& operators) { | ||
| bool IsUnaryOperator(TCppScope_t scope) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: redundant explicit casting to the same type 'Expr *' as the sub-expression, remove this casting [readability-redundant-casting]
| bool IsUnaryOperator(TCppScope_t scope) { | |
| Expr *DefaultArgExpr = PI->getDefaultArg(); |
Additional context
llvm/include/clang/AST/Decl.h:1809: source type originates from the invocation of this method
Expr *getDefaultArg();
^| switch (FD->getOverloadedOperator()) { | ||
| #define OVERLOADED_OPERATOR(Name, Spelling, Token, Unary, Binary, \ | ||
| MemberOnly) \ | ||
| case OO_##Name: \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
auto *D = (clang::Decl *)method;
^| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
auto *D = (clang::Decl *)func;
^| if (auto* DC = llvm::dyn_cast_or_null<DeclContext>(D)) { | ||
| ASTContext& C = getSema().getASTContext(); | ||
| DeclContextLookupResult Result = | ||
| DC->lookup(C.DeclarationNames.getCXXOperatorName( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: Called C++ object pointer is null [clang-analyzer-core.CallAndMessage]
return PI->getNameAsString();
^Additional context
lib/Interpreter/CppInterOp.cpp:3299: 'PI' initialized to a null pointer value
clang::ParmVarDecl* PI = nullptr;
^lib/Interpreter/CppInterOp.cpp:3301: Assuming null pointer is passed into cast
if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionDecl>(D))
^lib/Interpreter/CppInterOp.cpp:3301: 'FD' is null
if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionDecl>(D))
^lib/Interpreter/CppInterOp.cpp:3301: Taking false branch
if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionDecl>(D))
^lib/Interpreter/CppInterOp.cpp:3303: Assuming null pointer is passed into cast
else if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionTemplateDecl>(D))
^lib/Interpreter/CppInterOp.cpp:3303: 'FD' is null
else if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionTemplateDecl>(D))
^lib/Interpreter/CppInterOp.cpp:3303: Taking false branch
else if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionTemplateDecl>(D))
^lib/Interpreter/CppInterOp.cpp:3306: Called C++ object pointer is null
return PI->getNameAsString();
^8277fe9 to
fb19bfc
Compare
| OP_Coawait, | ||
| }; | ||
|
|
||
| enum OperatorArity { kUnary = 1, kBinary, kBoth }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| enum OperatorArity { kUnary = 1, kBinary, kBoth }; | |
| enum OperatorArity { kBoth = 0, kUnary, kBinary }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not understand this.
All Unary operators are not Binary operators. So kBoth = 0 and kUnary = 0 is confusing. Example ~ operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, my bad, fixed the suggestion.
| void GetBinaryOperator(TCppScope_t scope, enum BinaryOperator op, | ||
| std::vector<TCppFunction_t>& operators); | ||
| ///\returns true if scope is a unary operator | ||
| bool IsUnaryOperator(TCppScope_t scope); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| bool IsUnaryOperator(TCppScope_t scope); | |
| bool IsUnaryOperator(TCppFunction_t op); |
| bool IsUnaryOperator(TCppScope_t scope); | ||
|
|
||
| ///\returns true if scope is a binary operator | ||
| bool IsBinaryOperator(TCppScope_t scope); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| bool IsBinaryOperator(TCppScope_t scope); | |
| bool IsBinaryOperator(TCppFunction_t op); |
lib/Interpreter/CppInterOp.cpp
Outdated
| if (auto* FD = llvm::dyn_cast<FunctionDecl>(D)) { | ||
| if (FD->isOverloadedOperator()) { | ||
| switch (FD->getOverloadedOperator()) { | ||
| #define OVERLOADED_OPERATOR(Name, Spelling, Token, Unary, Binary, \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That type of check should be disabled in our clang-tidy config..
| if (auto* FD = llvm::dyn_cast<FunctionDecl>(D)) { | ||
| if (FD->isOverloadedOperator()) { | ||
| switch (FD->getOverloadedOperator()) { | ||
| #define OVERLOADED_OPERATOR(Name, Spelling, Token, Unary, Binary, \ | ||
| MemberOnly) \ | ||
| case OO_##Name: \ | ||
| return Unary; | ||
| #include "clang/Basic/OperatorKinds.def" | ||
| default: | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should move that into a utility function, write the switch such that it returns
OperatorArity getOperatorArity(...) {
if (Unary && Binary) return kBoth; ...
}
fb19bfc to
2c83c8d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
| OP_Coawait, | ||
| }; | ||
|
|
||
| enum OperatorArity { kNone, kUnary, kBinary, kBoth }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: enum 'OperatorArity' uses a larger base type ('unsigned int', size: 4 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]
enum OperatorArity { kNone, kUnary, kBinary, kBoth };
^
lib/Interpreter/CppInterOp.cpp
Outdated
| } | ||
| } | ||
| } | ||
| return kNone; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be unreachable, right? What does it mean arity::kNone? An operator without arguments? I do not think that's possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, should be unreachable.
It is for cases when the argument is something other than an operator or a function that is not an operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't that be an assert or an early exit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will say this is simpler. Assert will crash the program. In this case, the user can handle kNone as an error.
Also if I remove the return, clang-tidy will get angry.
Let me know if you still think we should remove kNone. I can do that, but not sure how I would satisfy clang-tidy's message of Non-void function does not return a value in all control paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should remove it. You can return ~0U
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compiler error:
/home/vipul-cariappa/Workspace/cpp-py/compiler-research/CppInterOp/lib/Interpreter/CppInterOp.cpp:3339:12: error: invalid conversion from ‘unsigned int’ to ‘Cpp::OperatorArity’ [-fpermissive]
3339 | return ~0U;
| ^~~
| |
| unsigned intShould I rename it to kInvalid instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cast it to OperatorArity?
lib/Interpreter/CppInterOp.cpp
Outdated
| if (auto* FD = llvm::dyn_cast<Decl>(D)) | ||
| operators.push_back(FD); | ||
| for (auto* i : Result) { | ||
| if (kind & getOperatorArity(i)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (kind & getOperatorArity(i)) | |
| if (kind == getOperatorArity(i)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When kind = kBoth should we return Unary Binary or Unary Binary?
This logic using & returns Unary Binary.
Whereas == will return Unary Binary.
Operators like - is both Unary and Binary.
I set the enum OperatorArity values so that I can perform this bitwise operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I say kBoth, I expect all unary and binary operators to be present in that list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, we should be using &. kUnary = 0b001 and kBinary = 0b010 and kBoth = kUnary ^ kBinary = 0b011.
No changes are required here.
2c83c8d to
ce7d778
Compare
|
Do we know why the check if failing? |
Looks like the API update in cppyy-backend was not pulled, rerunning now. should be green |
ce7d778 to
b32d28b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
| } | ||
| } | ||
| } | ||
| return (OperatorArity)~0U; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: The value '4294967295' provided to the cast expression is not in the valid range of values for 'OperatorArity' [clang-analyzer-optin.core.EnumCastOutOfRange]
return (OperatorArity)~0U;
^Additional context
include/clang/Interpreter/CppInterOp.h:88: enum declared here
enum OperatorArity { kUnary = 1, kBinary, kBoth };
^lib/Interpreter/CppInterOp.cpp:3319: Assuming 'D' is not a 'CastReturnType'
if (auto* FD = llvm::dyn_cast<FunctionDecl>(D)) {
^lib/Interpreter/CppInterOp.cpp:3319: 'FD' is null
if (auto* FD = llvm::dyn_cast<FunctionDecl>(D)) {
^lib/Interpreter/CppInterOp.cpp:3319: Taking false branch
if (auto* FD = llvm::dyn_cast<FunctionDecl>(D)) {
^lib/Interpreter/CppInterOp.cpp:3338: The value '4294967295' provided to the cast expression is not in the valid range of values for 'OperatorArity'
return (OperatorArity)~0U;
^resolving unary operators and operators defined within namespaces
b32d28b to
6cf8923
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
`test03_stllike_preinc` fixed by compiler-research/CppInterOp#401 others fixed by compiler-research/CppInterOp#394
`test03_stllike_preinc` fixed by compiler-research/CppInterOp#401 others fixed by compiler-research/CppInterOp#394
Description
Resolving unary operators and operators defined within namespaces.
Required for
+op instd::string.Fixes (issue)
Three issues at cppyy. Requires a patch for both CPyCppyy and cppyy-backend.
Type of change
Please tick all options which are relevant.
Testing
I have added in tests cases for unary operator lookup and lookup of operator within a namespace.
Checklist